AVRO-2079: Add ability to generate Java8 native date/time classes (new) by pvorb · Pull Request #309 · apache/avro

pvorb · 2018-05-04T12:07:18Z

As there was no progress on #248 for the past few months, I went ahead and resolved the merge conflicts of that PR and also fixed the remarks from @Kuroshii.

Here's a brief overview of what I've changed:

Reorder imports in TestJava8TimeConversions.java
Directly cast epochDays to int
Directly cast millis to int
Explicitly format java8 and joda time dates in test
Rename DateTimeLogicalTypeType to DateTimeLogicalTypeImplementation
Refactor registration of conversions
Make DateTimeLigicalTypeImplementation field final
Use parent POM version in Maven plugin tests
Log warning message about unknown parameter values
Remove unused imports

I would also suggest to rename the enum value JAVA8 and all of its uses to JSR310 because that's more specific to the java.time.* APIs introduced in Java 8 (see https://jcp.org/en/jsr/detail?id=310). I can add that change if you agree.

@Kuroshii I would appreciate if you could review this PR again, so maybe we'll have JSR-310 support in the next Avro release. Thanks!

lukejackson · 2018-05-08T00:21:36Z

Awesome, thanks @pvorb. I was not far off doing the same, as I am also very keen to see this feature added.

lukejackson · 2018-05-08T00:23:31Z

 import org.joda.time.LocalTime;

-public class TimeConversions {
+public class JodaTimeConversions {


I think it makes sense to rename this class, however in this comment Doug Cutting states:

The more back-compatible this is, the easier it will be to include in releases. Currently this renames an existing class, which is not back-compatible. As such, it thus could not be included in a minor, bugfix release.

I assume he is referring to this class when he says this.

lukejackson · 2018-05-08T00:27:00Z

    this.enableDecimalLogicalType = enableDecimalLogicalType;
  }

+  public boolean useJodaForDateTimeLogicalTypes() {


any reason not to expose the enum here? the two getters are not independent, and the interface essentially exposes four possible states when there are only two.

lukejackson · 2018-05-08T00:29:30Z

+    <artifactId>avro-parent</artifactId>
+    <groupId>org.apache.avro</groupId>
+    <version>1.9.0-SNAPSHOT</version>
+    <relativePath>../../../../../../../../../</relativePath>


relativePath should point to a specific pom.xml file, rather than a directory, here and in other pom.xml files touched by this change.

lukejackson · 2018-05-08T00:30:05Z

+1 on renaming to JSR310. (Note I am not an Avro committer).

lukejackson · 2018-05-08T12:37:44Z

@@ -163,9 +163,9 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or
 #if ($this.hasLogicalTypeField($schema))
  protected static final org.apache.avro.Conversions.DecimalConversion DECIMAL_CONVERSION = new org.apache.avro.Conversions.DecimalConversion();
 #if ($this.useJodaForDateTimeLogicalTypes())


i think this needs to be refactored to use the new getDateTimeLogicalTypeImplementation() method

DolgopolovOleg · 2018-05-17T15:29:57Z

Sorry guys, it's really crucial feature for us. What do you think how long will it take to approve it?
Many thanks =)

pvorb · 2018-05-22T21:53:30Z

Recent changes:

added a commit that replaces JAVA8 by JSR310 as suggested by me earlier in this thread. If anybody from the Avro project disagrees with that change, I will remove it.
removed the runtime dependency on Guava's Strings class from the AbstractAvroMojo, which caused the archetype modules to fail.
added a new entry in the CHANGES.txt describing the feature

pvorb · 2018-05-23T09:21:06Z

@nandorKollar Just saw that you want to remove CHANGES.txt in #310, so I removed the new entry in order to prevent a merge conflict.

- Reorder imports in TestJava8TimeConversions.java - Directly cast epochDays to int - Directly cast millis to int - Explicitly format java8 and joda time dates in test - Rename DateTimeLogicalTypeType to DateTimeLogicalTypeImplementation - Refactor registration of conversions - Make DateTimeLigicalTypeImplementation field final - Use parent POM version in Maven plugin tests - Log warning message about unknown parameter values - Remove unused imports

- Avoid renaming class TimeConversions - Set relativePath to pom files - Replace boolean getters with enum getter to better reflect internal state

pvorb · 2018-06-01T13:37:31Z

@nandorKollar Is there anything else I can do?

pvorb · 2018-06-27T12:04:12Z

@cutting Can you please have a look? Is there anything that needs to be changed before merging this request? It would be nice to get some feedback given the time me and others invested into it.

rocketraman · 2018-06-28T14:20:43Z

+1 java.time support is long overdue. Stephen Colebourne, the creator of joda-date, and contributor to java.time recommended migrating in 2014: http://blog.joda.org/2014/11/converting-from-joda-time-to-javatime.html.

pvorb · 2018-09-01T14:04:43Z

This PR and its precursor are by far the most upvoted pull requests for this project. Probably it’s worth considering to merge it?

malcolmrobbins · 2018-09-12T10:54:24Z

Hi pvorb,
we're really keen to get hold of Java8 time support within AVRO, even if this means creating our own fork for a while as we're about to undertake a significant project... What would it take for me to do this ? i.e. can I pull the changes and apply this to my own fork of the code? Alternatively is there an AVRO release imminent to save me the trouble?

malcolmrobbins · 2018-09-13T07:39:36Z

i.e. can I get hold of vorb:use-java8-time for our "private use" until it becomes generally available

rocketraman · 2018-09-13T15:16:26Z

LOL: https://issues.apache.org/jira/browse/AVRO-2210

pvorb · 2018-09-13T16:49:59Z

@malcolmrobbins Feel free to do this. Don't expect an official release with this change anytime soon.

malcolmrobbins · 2018-09-14T04:17:46Z

Thanks for that - I took a copy of your branch and tried a mvn test install and the build failed on Apache Avro Tools with an error that indicates a dependency on avro-mapred:jar:tests could not be satisfied.
Note that building mapred worked fine but unlike the other modules it didn't build a tests jar and this seems to be the root cause of the dependency error.
The actual error was:

[ERROR] Failed to execute goal on project avro-tools: Could not resolve dependencies for project org.apache.avro:avro-tools:jar:1.9.0-SNAPSHOT: Could not find artifact org.apache.avro:avro-mapred:jar:tests:1.9.0-SNAPSHOT -> [Help 1]

Do have any pointers as to how I can overcome this? Thanks in advance.

malcolmrobbins · 2018-09-17T01:32:59Z

FYI I overcame this problem. To do this I simply needed to run the build.sh script in the lang/java folder rather than following the more explicit mvn instructions in the How To Contribute guide. Now I just need to figure out how to get the avro-maven-plugin to generate Java sources that use Java 8 datetime rather than Joda

kgalieva · 2018-09-18T19:54:51Z

@malcolmrobbins You need to add <dateTimeLogicalTypeImplementation>jsr310</dateTimeLogicalTypeImplementation> into configuration section.

@pvorb thank you! This patch works fine on our schemas!

Fokko · 2018-09-24T15:02:43Z

I've ran some tests against Divolte and it looks great. Can we get this merged?

nandorKollar · 2018-09-25T14:11:36Z

  }

+  /**
+   * Creates a specific compler with the default (Joda) type for date/time related logical types.


nit: compler -> compiler

nandorKollar

Looks good to me, I just ask for two minor changes.

If nobody has any objection against merging this, then after waiting a couple of days for feedback I'll merge to master.

pvorb · 2018-09-25T15:02:34Z

@nandorKollar I changed the code according to your requests. Good spot about overriding the getRecommendedSchema() methods in each conversion!

nandorKollar · 2018-10-01T10:08:12Z

Merged to master, thanks @aukevanleeuwen and @pvorb for working on this!

pvorb · 2018-10-02T12:59:13Z

Thanks, @nandorKollar!

Blackdread · 2019-01-04T15:35:51Z

Hi, it is merged but avro maven plugin 1.8.0 still does not support that, right?
I am trying to set my avsc file but still get jodatime once generated

nandorKollar · 2019-01-04T16:23:59Z

@Blackdread correct, it is not yet released. Java 8 native date/time support will be available in 1.9.0.

Haybu · 2019-04-18T00:24:12Z

Any rough estimation when 1.9.0 will be released with this feature?

Fokko · 2019-04-21T10:33:11Z

Next week I'll cut the branch and release the RC. Please subscribe to the dev maillist so you can help test the RC 👍

…w) (apache#309) AVRO-2079: Add ability to use Java 8 date/time types instead of Joda time. Add compiler option to allow generating Java 8 date/time classes instead of Joda time. This commit adds a new optional compiler option -dateTimeLogicalTypeImpl to specify which date/time implementation to use. It could be joda or jsr310, if not specified by, then the default is joda.

pvorb force-pushed the use-java8-time branch from 6b23527 to 6c3921c Compare May 4, 2018 13:44

lukejackson reviewed May 8, 2018

View reviewed changes

pvorb mentioned this pull request May 17, 2018

AVRO-2079: Add ability to generate Java8 native date/time classes #248

Closed

nandorKollar reviewed May 17, 2018

View reviewed changes

Comment thread lang/java/tools/src/main/java/org/apache/avro/tool/SpecificCompilerTool.java

pvorb force-pushed the use-java8-time branch from b72e623 to c6257f2 Compare May 22, 2018 21:44

pvorb force-pushed the use-java8-time branch from c6257f2 to adf9bbd Compare May 23, 2018 09:19

aukevanleeuwen and others added 6 commits May 28, 2018 14:52

AVRO-2079: Add ability to generate Java8 native date/time classes

e17b001

AVRO-2079: Incorporate remarks from @lukejackson

5041bdf

- Avoid renaming class TimeConversions - Set relativePath to pom files - Replace boolean getters with enum getter to better reflect internal state

AVRO-2079: Use correct method in record template

b6c06a9

AVRO-2079: Rename logical type impl JAVA8 to JSR310

1f4cf54

AVRO-2079: Remove dependency on Guava from Mojo

902b647

pvorb force-pushed the use-java8-time branch from adf9bbd to 902b647 Compare May 28, 2018 12:53

lukejackson mentioned this pull request Aug 18, 2018

[AVRO] Generation of Avro schemas with logical types. FasterXML/jackson-dataformats-binary#133

Closed

nandorKollar reviewed Sep 25, 2018

View reviewed changes

Comment thread lang/java/avro/src/main/java/org/apache/avro/data/Jsr310TimeConversions.java

nandorKollar reviewed Sep 25, 2018

View reviewed changes

AVRO-2079: Define recommended schemas

d8f0812

nandorKollar approved these changes Sep 27, 2018

View reviewed changes

nandorKollar merged commit b814cd0 into apache:master Oct 1, 2018

kgalieva mentioned this pull request Oct 3, 2018

Improved conversions handling + pluggable conversions support [AVRO-1891, AVRO-2065] #329

Merged

Uh oh!

Conversation

pvorb commented May 4, 2018

Uh oh!

lukejackson commented May 8, 2018

Uh oh!

lukejackson May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukejackson May 8, 2018

Choose a reason for hiding this comment

Uh oh!

lukejackson May 8, 2018

Choose a reason for hiding this comment

Uh oh!

lukejackson commented May 8, 2018

Uh oh!

lukejackson May 8, 2018

Choose a reason for hiding this comment

Uh oh!

pvorb May 8, 2018

Choose a reason for hiding this comment

Uh oh!

DolgopolovOleg commented May 17, 2018

Uh oh!

Uh oh!

pvorb commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvorb commented May 23, 2018

Uh oh!

pvorb commented Jun 1, 2018

Uh oh!

pvorb commented Jun 27, 2018

Uh oh!

rocketraman commented Jun 28, 2018

Uh oh!

pvorb commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malcolmrobbins commented Sep 12, 2018

Uh oh!

malcolmrobbins commented Sep 13, 2018

Uh oh!

rocketraman commented Sep 13, 2018

Uh oh!

pvorb commented Sep 13, 2018

Uh oh!

malcolmrobbins commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malcolmrobbins commented Sep 17, 2018

Uh oh!

kgalieva commented Sep 18, 2018

Uh oh!

Fokko commented Sep 24, 2018

Uh oh!

Uh oh!

nandorKollar Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

nandorKollar left a comment

Choose a reason for hiding this comment

Uh oh!

pvorb commented Sep 25, 2018

Uh oh!

nandorKollar commented Oct 1, 2018

Uh oh!

pvorb commented Oct 2, 2018

Uh oh!

Blackdread commented Jan 4, 2019

Uh oh!

nandorKollar commented Jan 4, 2019

Uh oh!

Haybu commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fokko commented Apr 21, 2019

Uh oh!

lukejackson May 8, 2018 •

edited

Loading

pvorb commented May 22, 2018 •

edited

Loading

pvorb commented Sep 1, 2018 •

edited

Loading

malcolmrobbins commented Sep 14, 2018 •

edited

Loading

Haybu commented Apr 18, 2019 •

edited

Loading